Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Jan 7, 2020

This allow collecting state cache requests and report cache hit ratio
@arkpar

During the initial sync hit ratio seems to be close to 100%, by the way :)

@parity-cla-bot
Copy link

It looks like @NikVolf signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@NikVolf NikVolf added the A0-please_review Pull request needs code review. label Jan 7, 2020
Copy link
Contributor

@marcio-diaz marcio-diaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few very little nits. I'll take a deeper look tomorrow.

@arkpar
Copy link
Member

arkpar commented Jan 10, 2020

I'd rename usage.rs to usage_stats.rs or simply stats.rs

@gavofyork gavofyork merged commit bd540a6 into master Jan 13, 2020
@gavofyork gavofyork deleted the nv-io-stats-sdb branch January 13, 2020 08:10
let mut bytes: u64 = 0;
for (key, (val, rc)) in operation.db_updates.drain() {
if rc > 0 {
ops += 1;
Copy link
Contributor

@cheme cheme Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field db_updates are calculated trie nodes delta that are going to be written in rocksdb, so I think we should change the name, because it gives the wrong idea that the collected read stats (key value) are comparable with those writes (trie nodes).
We can also get write stats that correspond to our read, that would be field 'storage_updates' and 'child_storage_updates', IIRC those are send to storage cache for update and are complete in term of delta.
But we could also hook some 'write_cache' stat in state-machine overlay-db to compare what are actual post block modif ('storage_updates' fields) against what update the runtime did call. That way, we get an idea of the proportion of updates that are done on a same key. (at

let entry = self.prospective.top.entry(key).or_default();
and the child one)

} else if rc < 0 {
ops += 1;
bytes += key.len() as u64;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could store that in a different write counter.
With non archive mode it is actually an additional memory consumption until pruning, but with archive mode it is a no-op.
Or we check mode (it is maybe accessible in client-db).

let value = self.state.child_storage(storage_key, child_info, &key.1[..])?;

// just pass it through the usage counter
let value = self.usage.tally_child_key_read(&key, value, false);
Copy link
Contributor

@cheme cheme Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have something like state_read_cache - state_cache = state-read-db-backend.
One thing that we do not register regarding read query emitted from the runtime will be all scenario such as :

  • querying a key that has been change during block processing: in this case the overlay-db from state machine return the value and we do not have stats.
  • that's all I can think off :)

We could register them in

let result = self.overlay.storage(key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
I think it should be register as a different cache and added to the total read as it is done for storage cache.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants